Repository navigation
[CXH-3052] Run the pr-review tests in CI, and move the pr-review detail out of the README - #155
Conversation
…EADME Three loose ends from #147 and #149, none of which depend on how the reviewer clears its blocks. Nothing ran this repository's tests. verify.yaml is a reusable workflow that tests a CALLER repo's checkout, so `make test` existed here but only ever ran by hand. That left the pr-review scripts — the most logic in the repo, and ~1200 lines newer than the rest of it after #149 — with 175 automated tests and no automated run. It is not a hypothetical gap. While working on #147 I shipped two defects into action.yml that these tests would have caught at once: a `bash -eo pipefail` case where `var=$(failing-cmd)` aborted the step instead of falling through to its fallback, and a doc pointer broken by moving a file. Both were found by a reviewer afterwards. So: add the pr-review suite to `make test-scripts`, and add self-test.yaml to run `make test` on pull requests and pushes to main. The suite passes as-is — verified end to end locally, exit 0 — so this starts green rather than landing a red check for someone else to fix. yq is installed explicitly because three script tests shell out to it and fail hard without it; current runner images happen to ship it, and depending on that quietly is how this breaks later. Separately, the README: PR Review was 147 of its 475 lines, and @gontzess asked in #147 for it to move. The other workflows already do this — docs/release-workflow.md, docs/verify-workflow.md, docs/connector-docs-verify.md hold the internals while the README keeps a paragraph, a pointer and how to call the thing. PR Review was the only one carrying all its detail inline. The moved text is byte-identical after demoting its headings one level, so that part of the diff reads as a move. README is now 353 lines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Superseded — see the current review report for commit
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Superseded — see the current review report for commit
|
The fallback downloaded a release binary and installed it to /usr/local/bin with sudo, verifying nothing. A release tag can be repointed, so pinning the tag does not say what ends up executing in CI. Pin the digest too: SHA-256 of yq_linux_amd64 taken from the v4.44.3 `checksums` release asset, not from the copy I downloaded — cross-checked against a local download, which matched. Verification happens before the binary is made executable, and `install -m 0755` replaces the mv + chmod. Fail-closed confirmed under the shell Actions actually uses (`bash --noprofile --norc -eo pipefail`): a wrong digest exits 1 and never reaches the install line. My first attempt to test that was wrong — the subshell hung off an `&&`, which suppresses `set -e` — and appeared to pass while failing open. This path only runs if a future runner image stops shipping yq, which is exactly when nobody will be watching it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
Review of the first commits found the gate was narrower and noisier than it claimed. make test-go enumerated five packages by hand. cmd/release-metadata has nine tests and was not among them, while two packages that were listed have no tests at all — the list was stale in both directions. So the PR whose premise is "nothing runs this repo's tests" was shipping a green check that silently skipped nine. `go test ./...` closes it and keeps future packages covered by default. Verified the omitted package passes under `env -i` with no git identity, which is what the runner has. The pr-review scripts print `::error::` and `::warning::` as their normal failure paths, and the tests drive those paths as SUCCESS cases: a passing run emitted 9 errors and 3 warnings at column zero, which the runner parses as workflow commands rather than text. Every green self-test would have annotated its own PR with red errors. `unittest -b` buffers passing tests' output and fixes the green case at the source — output drops from hundreds of lines to 39, which also makes a real failure findable. A failing test still replays its captured output, so the step also brackets `make test` in ::stop-commands::. The yq step's verified-download branch never executed: the runner image ships yq, so `command -v` always succeeded and the pinned v4.44.3 binary was dead code — 22 lines explaining a path that does not run, while the suite actually used the image's unpinned 4.54. That is the cost of pinning with none of the guarantee. Replaced by asserting presence: yq comes from the image like python3 and everything else, and if that ever changes this fails loudly here naming the tool rather than with a bare "command not found" mid-suite. Concurrency fell back to github.ref on push, so two merges to main shared one group and the first was cancelled, leaving that commit untested. Now github.run_id, which is the shape pr-review.yaml and general-pr-review.yaml already use. Added timeout-minutes: 15 — the job runs PR-head code by design, and the suite takes seconds, so the 360-minute default is all downside. Docs: the ANTHROPIC_API_KEY paragraph was copied rather than moved, leaving two copies of the one instruction with a security consequence. It now lives in the README only. The input table says it describes pr-review.yaml's inputs (general-pr-review.yaml declares none) and carries the summary_marker constraint the action hard-fails on. README's Development section says what `make test` now gates, what running it locally needs, and that verify.yaml is not this. Not added, deliberately: golangci-lint and `make workflow-validate`. The same argument covers them, but neither has run against this repo and adding an unproven job would contradict this PR starting green. Noted in the workflow. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
FeliLucero1
changed the title
Superseded — see the current review report for commit
|
|
Heads up @gontzess — this PR just went red, and it is not this PR's change. Turning Both failures are the same line shape: json.loads(subprocess.check_output(["yq", ".", str(ROOT / ".github/workflows/release.yaml")], text=True))
# json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)
Confirmed it is pre-existing rather than mine: I have not touched it. It is two days old, it is yours, and it is part of the incident work, so the call on how to fix it is yours — In the meantime this PR's own check stays red, which I would rather leave visible than hide: a green |
Superseded — see the current review report for commit
|
sergiocorral-conductorone
left a comment
sergiocorral-conductorone
left a comment
There was a problem hiding this comment.
Automated review. drafted 4 → confirmed 4, low-confidence 0, dropped 0, added 0 (panel: opus-5.5 / gpt-6-astra / gemini-4-argon, all high). The Makefile and docs moves look good. The new self-test.yaml fails its own check and duplicates ci.yaml. Linked ticket CXH-3052 body could not be read, so its Fix-list coverage was not checked.
|
Correction to my comment above, and apologies for the noise @gontzess — The apt What actually failed was my I also had the history wrong. I said nothing ran Removing |
@sergiocorral-conductorone caught that this duplicates .github/workflows/ci.yaml ("Repository CI"), which runs go vet ./... , go test ./... and make test-scripts on pull requests and pushes to main. Since this branch adds the pr-review suite to make test-scripts, ci.yaml's shell-tests job already runs it — and did, passing on this PR's own head while self-test failed. The premise this branch opened with was true when it was written and stopped being true the same day: ci.yaml landed in e9b91a2 yesterday at 18:05 as part of the CA-incident batch, after this PR existed. I merged main and did not re-check it. Dropping the workflow also removes the yq problem it had, rather than fixing it. The apt yq package is kislyuk's Python/jq build, which is what the script tests need; ci.yaml installs that explicitly and puts it first on PATH, while self-test.yaml asserted only that *some* yq existed and got the runner image's mikefarah build. That was the failure I reported as a pre-existing break in test-tool-integrity.py — it was not one, and the comment is corrected on the PR. What remains is the part that was genuinely missing: the Makefile line, which ci.yaml now runs. The README says which yq is required and why, since getting that wrong is exactly what cost this branch a round. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Superseded — see the current review report for commit
|
FeliLucero1
changed the title
Superseded — see the current review report for commit
|
|
On the one open suggestion in the latest report — the description was already rewritten, in the same sitting as
So the finding was accurate when the run read it and is not now. Noting it rather than leaving a stale "Suggestions: 1" for whoever picks this up. This is the third time in this series that a description edit landed just after the run that read it. Not the re-post problem #156 fixes — that one is about threads — and probably not worth fixing: it is inherent to editing a description after pushing. Worth knowing when reading a report, though. |
sergiocorral-conductorone
left a comment
sergiocorral-conductorone
left a comment
There was a problem hiding this comment.
Automated review (pipeline test run, ticket CXH-3052 pre-fetched). drafted 3 → confirmed 1, low-confidence 2, dropped 0, added 0 (panel: opus / gpt-6-astra / gemini-4-argon, all high).
`go test ./...` ran cmd/ca-trust-guard twice: once unrestricted, then again under GOTOOLCHAIN=local GOPROXY=off. The first run warms the module and toolchain cache, so the hermetic second run stops proving anything. Restore the explicit list and add ./cmd/release-metadata, which has tests and was the one package the list was actually missing. Also correct the README: ci.yaml does not call `make`. Its go-tests job runs `go vet ./...` and `go test ./...` directly; shell-tests runs `make test-scripts`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
The hand-maintained list had been missing ./cmd/generate-windows-manifest and ./pb/artifacts/v1 since before this PR, so `make test-go` never even compiled them. Two of the packages it did name have no tests at all, which says the list was always meant to be "compile everything, test what has tests" — it had just drifted. Derive it from `go list` minus ca-trust-guard instead. That package still gets exactly one run, the hermetic one, and a new package can no longer be skipped by forgetting to add it here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Superseded — see the current review report for commit
|
General PR Review: [CXH-3052] Run the pr-review tests in CI, and move the pr-review detail out of the READMEBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryThis PR adds the pr-review Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. Resolved prior findings
Reviewed commit: |
sergiocorral-conductorone
left a comment
sergiocorral-conductorone
left a comment
There was a problem hiding this comment.
Automated review. LGTM — addressed: ca-trust-guard now runs once and offline only (explicit package list), README now says which ci.yaml jobs run which targets, and the self-test.yaml-related findings are moot since that workflow was removed. No unresolved review threads remain. Note: CXH-3052's Linear text still mentions self-test.yaml (stale wording only).
Two loose ends from #147 and #149. The workflow this PR originally added is gone — see below.
The pr-review suite was in no test target
.github/actions/pr-review/scripts/test_*.pywas in no Makefile target and no workflow, so the scripts carrying the most logic in this repo — and ~1200 lines newer than the rest after #149 — had 175 automated tests and no automated run.Not hypothetical: while working on #147 two defects shipped into
action.ymlthat these tests would have caught at once — abash -eo pipefailcase wherevar=$(failing-cmd)aborted the step instead of falling through to its fallback, and a doc pointer broken by moving the file it pointed at. Both were found by a reviewer afterwards.This adds one line to
make test-scripts.ci.yaml'sshell-testsjob runs that target, so the suite is now covered on every PR and push tomainthat touches.github/actions/**,cmd/**,scripts/**or theMakefile.What changed during review. This PR originally added a
self-test.yamlworkflow, on the premise that nothing ranmake testhere. That was true when it was written and stopped being true the same day:ci.yaml("Repository CI") landed ine9b91a2as part of the CA-incident batch, after this PR was opened. @sergiocorral-conductorone caught the duplication. The workflow is dropped; the Makefile line is the part that was actually missing.That also retired a bug of mine.
self-test.yamlasserted that someyqwas installed and got the runner image's mikefarah build, while the script tests need kislyuk's Python/jq one — whichci.yamlinstalls explicitly. I reported that failure as a pre-existing break intest-tool-integrity.py; it was not one, and that is corrected in a comment above.The README
@gontzess asked for this in #147 and was right: PR Review was 147 of the README's 475 lines. The other workflows already follow the split —
docs/release-workflow.md,docs/verify-workflow.md,docs/connector-docs-verify.mdcarry the internals while the README keeps a paragraph, a pointer, and how to call the thing. PR Review was the only one with everything inline.The moved text is byte-identical after demoting its headings one level — nothing reworded, nothing dropped — so that part of the diff reads as a move. I verified that programmatically, not by eye.
The Development section now also names which
yqis required and why, since getting that wrong is what cost this branch a round.Not included
make workflow-validate.ci.yamlalready runs shellcheck; extending it is a separate call.license-shapes.mddiscarding anok— different repo, ConductorOne/claude-marketplace#211.🤖 Generated with Claude Code